[grid] Eliminate vendor-specific handling of extension capabilities #14485

sbabcoc · 2024-09-10T18:15:00Z

User description

Description

The existing handling of extension capabilities assigns special significance to four recognized prefixes: goog:, moz:, ms:, and se:. Capabilities with these prefixes are entirely ignored by the current default slot matcher implementation, but custom prefixes are considered, as well as those for Safari and Appium. This inconsistency means that properties in extension capabilities can cause affected newSession requests to fail even though extension capabilities are supposed to be vendor-specific and should probably not be evaluated by the default slot matcher.

This PR retains the "special" status of the "se:" prefix and specifically filters out all extension capabilities with names that end with "options", "Options", "loggingPrefs", and "debuggerAddress". This maintains existing behavior while allowing node configurations and newSession requests to include extension capabilities that won't affect slot matching.

Motivation and Context

Revolves #14461

Note that this is technically a breaking change, because it revises the default slot matcher to consider "special" extension capabilities without name suffix "options" that would previously have been ignored, and the matcher will now ignore "non-special" extension capabilities with name suffix "options" that would previously have been considered.
However, I'm unaware of any current use cases that will be adversely affected by this change.

The strategy employed by the PR is that extension capabilities which should be ignored for matching can be expressed as capabilities with name prefix "se:" or one of the "special" name suffixes. All other extension capabilities will be considered for slot matching. This is a generalization/extrapolation of existing patterns from current use cases.

Recommended slot matcher enhancements

To reduce the need for custom slot matchers, we could extend the WebDriverInfo interface to add a new method that vendors could implement to evaluate their own criteria:

Boolean matches(Capabilities stereotype, Capabilities capabilities);

The default implementation would return null to indicate that no evaluation has been performed. If the vendor chooses to implement the matches method, their initial step must be to invoke their own isSupporting method, returning null if the corresponding driver doesn't support the specified capabilities.

The implementation in DefaultSlotMatcher would be updated to iterate over the available WebDriverInfo providers, retaining only those whose isSupporting methods return true. The matches methods of this filtered list of providers will then be applied to the available nodes to determine which of these satisfies the criteria specified by the client request. The nodes that satisfy these evaluations (if any) would then be evaluated against the remaining common criteria.

Another potential enhancement would be to enable vendor-specific matches methods to return a weighted integer result instead of a simple Boolean. This would enable best-match behavior. Perhaps this is getting too complicated, though.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

PR Type

Bug fix, Tests

Description

Removed the special status of certain extension capability prefixes in DefaultSlotMatcher, opting to ignore all extension capabilities with names ending in "options" (case-insensitive).
Simplified the logic for matching capabilities by removing unnecessary checks and conditions.
Updated RelaySessionFactory to streamline session creation by removing redundant capability filtering.
Revised test cases in DefaultSlotMatcherTest to align with the new handling of extension capabilities.

Changes walkthrough 📝

Relevant files

Bug fix

DefaultSlotMatcher.java `Revise extension capabilities handling in DefaultSlotMatcher` java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java Removed special handling for specific extension capability prefixes. Updated logic to ignore all extension capabilities with names ending in "options" (case-insensitive). Simplified capability matching logic.	+31/-51
RelaySessionFactory.java `Simplify session creation in RelaySessionFactory` java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java Removed logic to filter out browserName when appium:app is present. Simplified session creation process.	+0/-11

Tests

DefaultSlotMatcherTest.java `Update DefaultSlotMatcher tests for revised capability handling` java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java Updated test cases to reflect changes in extension capability handling.	+10/-8

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

qodo-merge-pro · 2024-09-10T18:15:29Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Key issues to review Potential Performance Issue The new implementation of `initialMatch` method uses `reduce(Boolean::logicalAnd)` which may not short-circuit on the first false condition. Consider using `allMatch` for better performance. Possible Bug The `extensionCapabilitiesMatch` method now ignores complex objects and arrays, which might lead to unintended matching behavior for certain extension capabilities.

qodo-merge-pro · 2024-09-10T18:15:54Z

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Improve precision in matching complex capability values Consider using a more specific check for complex objects instead of returning `true` for all non-String, non-Number, and non-Boolean values. This could prevent unintended matches for complex objects. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [144-148] if (capabilities.getCapability(name) instanceof Number \|\| - capabilities.getCapability(name) instanceof Boolean) { + capabilities.getCapability(name) instanceof Boolean \|\| + capabilities.getCapability(name) instanceof Map) { return Objects.equals(stereotype.getCapability(name), capabilities.getCapability(name)); } -return true; +return false; Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion enhances the precision of the matching logic by returning false for unsupported complex objects, preventing unintended matches and improving the accuracy of the matching process.	8
Enhancement	Improve handling of complex capability values in extension capabilities matching Consider using `Map.of()` or `ImmutableMap.of()` instead of creating a new HashMap for the complex capability values. This can make the code more concise and potentially more efficient. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [144-148] if (capabilities.getCapability(name) instanceof Number \|\| - capabilities.getCapability(name) instanceof Boolean) { + capabilities.getCapability(name) instanceof Boolean \|\| + capabilities.getCapability(name) instanceof Map) { return Objects.equals(stereotype.getCapability(name), capabilities.getCapability(name)); } return true; Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies the need to handle complex capability values like Maps, which aligns with the changes in the test cases. This improves the robustness of the capability matching logic.	7
Enhancement	Add logging for skipped extension capabilities to improve debugging Consider adding a log statement when skipping extension capabilities to improve debugging and traceability of the matching process. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [88] -.filter(name -> !name.contains(":")) +.filter(name -> { + boolean isExtensionCapability = name.contains(":"); + if (isExtensionCapability) { + LOG.fine("Skipping extension capability: " + name); + } + return !isExtensionCapability; +}) Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding logging for skipped extension capabilities can aid in debugging and understanding the matching process, though it is not critical for functionality.	6
Best practice	Use a constant for the extension capability separator to improve code maintainability Consider using a constant for the ":" character used to identify extension capabilities. This would improve maintainability and reduce the risk of typos. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [88] -.filter(name -> !name.contains(":")) +private static final String EXTENSION_CAPABILITY_SEPARATOR = ":"; +// In the method: +.filter(name -> !name.contains(EXTENSION_CAPABILITY_SEPARATOR)) + Apply this suggestion Suggestion importance[1-10]: 5 Why: Using a constant for the separator improves maintainability and reduces the risk of errors, but it is a minor improvement in terms of code quality.	5

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java

java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java

java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java

java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java

java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java

diemol · 2025-01-31T20:59:07Z

java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java

-      filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String) null);
-    }
-
-    capabilities = capabilities.merge(filteredStereotype);


Now I remember. This was done because the user can specify something in the stereotype that will be sent to the Node that will receive the request.

diemol · 2025-01-31T20:59:48Z

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java

-   List of prefixed extension capabilities we never should try to match, they should be
-   matched in the Node or in the browser driver.
-  */
-  private static final List<String> EXTENSION_CAPABILITIES_PREFIXES =


Why is it an inconsistency? Browser vendors just need those capabilities to be passed along. They do not use the Grid and configure stereotypes to do matching.

Fixes #15481 Fixes #14485

qodo-merge-pro bot added P-bug fix PR addresses a known issue tests Review effort [1-5]: 3 labels Sep 10, 2024

sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from a82c998 to 14999b7 Compare September 10, 2024 18:15

sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from 14999b7 to dbe50be Compare September 10, 2024 18:41